-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add client queries to the relayer #38
Conversation
More work needs to be done here but at this point trying to figure out why we get error when we ask for proof vs. with no proof. Will add more soon. |
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=======================================
Coverage ? 7.9%
=======================================
Files ? 42
Lines ? 1299
Branches ? 174
=======================================
Hits ? 103
Misses ? 1163
Partials ? 33 Continue to review full report at Codecov.
|
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | ||
pub struct ClientState { | ||
id: String, | ||
trusting_period: Duration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note (mostly to myself):
in the go code, time.Duration
actually is this type alias:
// A Duration represents the elapsed time between two instants
// as an int64 nanosecond count. The representation limits the
// largest representable duration to approximately 290 years.
type Duration int64
while rust's std::time::Duration
actually is
/// A `Duration` type to represent a span of time, typically used for system
/// timeouts.
///
/// Each `Duration` is composed of a whole number of seconds and a fractional part
/// represented in nanoseconds. If the underlying system does not support
/// nanosecond-level precision, APIs binding a system timeout will typically round up
/// the number of nanoseconds.
/// [...]
pub struct Duration {
secs: u64,
nanos: u32, // Always 0 <= nanos < NANOS_PER_SEC
}
Same for the unbonding_period
field below.
So for deserialization (independent from amino or not) we need to translate between the two.
ref #45
id: String, | ||
trusting_period: Duration, | ||
unbonding_period: Duration, | ||
latest_header: Header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the Header here contains a SignedHeader
which in turn contains a Commit
. Looking at the Commit
struct in the iqlusion/relayer (on master) shows that they are already using tendermint v0.33.2
which modified the Commit
struct to:
type Commit struct {
// NOTE: The signatures are in order of address to preserve the bonded
// ValidatorSet order.
// Any peer with a block can gossip signatures by index with a peer without
// recalculating the active ValidatorSet.
Height int64 `json:"height"`
Round int `json:"round"`
BlockID BlockID `json:"block_id"`
Signatures []CommitSig `json:"signatures"`
// ...
}
while this branch uses the v0.32 version of the Commit:
pub struct Commit {
/// Block ID of the last commit
pub block_id: block::Id,
/// Precommits
pub precommits: Precommits,
}
Or in golang (from v0.32.x): https://github.com/tendermint/tendermint/blob/64e61365c16f74d56ccaa9eb784e05df5b01764c/types/block.go#L489-L494
v0.33 tendermint-rs is currently worked on in informalsystems/tendermint-rs#196
Does it make sense to assume we want to decode v0.33 commits directly (for the sake of #45)? @ancazamfir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, let's look at 0.33! thx!
…ormalsystems/ibc-rs into anca/add_consensus_query_cmd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. I did not review the cli in detail but everything else. LGTM
fn verify_client_consensus_state( | ||
&self, | ||
root: &CommitmentRoot, | ||
) -> Result<(), Self::ValidationError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the other state verification functions are not included on purpose?
https://github.com/cosmos/cosmos-sdk/blob/b5a658729170e577b0e6e809e73d5e0756b90e07/x/ibc/02-client/exported/exported.go#L26-L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a bit more work to get there :)
trusting_period: Duration, | ||
unbonding_period: Duration, | ||
latest_header: Header, | ||
frozen_height: crate::Height, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order in the go-code is different (latest header should be last):
frozen_height: crate::Height, | |
frozen_height: crate::Height, | |
latest_header: Header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oy! good catch! thx, just changed. The problem is that we cannot test properly with SDK since we don't deserialize yet. Once we move to protobuf and we do the decoding we will catch more of these.
id: String, | ||
trusting_period: Duration, | ||
unbonding_period: Duration, | ||
latest_header: Header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only commit batched together with the suggestion below (frozen_height):
latest_header: Header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
impl CommitmentProof { | ||
pub fn from_bytes(_bytes: &[u8]) -> Self { | ||
todo!() | ||
} | ||
} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we delete this but but extend the abci::Proof
type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but will do in a separate issue/PR
.build() | ||
.unwrap() | ||
.block_on(future) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably get rid of this block_on
method (might be relevant: informalsystems/tendermint-rs#171, informalsystems/tendermint-rs#173, informalsystems/tendermint-rs#125 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I have been unable to get abscissa_tokio
to work, abscissa_tokio::run
just hangs (on my machine at least).
I haven't really put any time into debugging this, as I'd rather wait until we figure the new light client + relayer architecture, before spending time on that (block_on
does the job just fine for now imho as it is called at the top level only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really put any time into debugging this, as I'd rather wait until we figure the new light client + relayer architecture, before spending time on that (block_on does the job just fine for now imho as it is called at the top level only).
Makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romac just a note that we use this now for the query also.
@@ -67,17 +74,15 @@ impl<CS> ConsensusStateResponse<CS> { | |||
pub fn new( | |||
client_id: ClientId, | |||
consensus_state: CS, | |||
abci_proof: abci::Proof, | |||
abci_proof: Option<abci::Proof>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't the type alias (CommitmentProof
) used consistently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, changed.
use crate::chain::Chain; | ||
use crate::error; | ||
|
||
pub struct QueryClientFullState<CLS> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the corresponding request to ClientStateResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, changed the name of response.
} | ||
|
||
fn amino_unmarshal_binary_length_prefixed<T>(_bytes: &[u8]) -> Result<T, error::Error> { | ||
todo!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is reasonable to collect outstanding comments in a followup issue and merge this as it is. LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* debugging the consensus query with proof * started on client state * fixed unused warning * add height param to client query * cargo fmt * make proof option for client state * fix client state prove flag * cleanup queries * Try integration with tm-rs v0.33 to pick the new merkle proof * add validation function for common params * add utils * some cleanup and err handling * addressing Ismail's comments
Closes: #37
Description
Add consensus state and full state query clis to the relayer:
Note: full deserialization does not yet work, waiting for migration of SDK/ TM to protobuf.